feat: scope CLI state files by API key to isolate concurrent accounts#44
Open
andreakiro wants to merge 1 commit into
Open
feat: scope CLI state files by API key to isolate concurrent accounts#44andreakiro wants to merge 1 commit into
andreakiro wants to merge 1 commit into
Conversation
Two different accounts running the CLI on the same machine used to share the same ~/.notte/cli/current_session (and current_agent, current_session_expiry, current_viewer_url) file. Switching API keys or running two shells under different accounts could cause one account's interactive `sessions stop` to target the other account's session. Stack on top of the non-TTY skip-write fix (#43) - that fix already removed the parallel-worker race; this PR closes the related multi-account collision. - Suffix the four state files with `.<sha256(api_key)[:8]>` whenever an API key is configured. When no key is available (e.g. during `notte auth login` itself) the suffix is empty and behavior is unchanged. - Read uses the scoped path first and falls back to the legacy unscoped path so users upgrading from an earlier CLI don't lose their existing current_session. - Clear removes both the scoped and the legacy unscoped file so upgraders don't see stale state. - Centralize the four "current_*" file helpers in state_scope.go (readStateFile / writeStateFile / clearStateFile). sessions.go and agents.go shrink as a result. - New tests cover env-var derivation, scoped-write, legacy-read fallback, scoped-over-legacy precedence, clear-removes-both, and isolation between two distinct API keys. The cmd test package pins the override to "" via TestMain so existing tests that hand-roll state paths continue to work unchanged.
|
| Filename | Overview |
|---|---|
| internal/cmd/state_scope.go | New file implementing per-API-key state file scoping with read/write/clear helpers; stateFilePath is defined but never called (dead code) |
| internal/cmd/state_scope_test.go | Comprehensive unit tests covering suffix derivation, scoped writes, legacy fallback, precedence, clear behaviour, and account isolation; TestMain correctly pins override to "" for the full package |
| internal/cmd/sessions.go | Refactored to delegate all four state-file operations to the new helpers; logic unchanged, net ~70 lines removed |
| internal/cmd/agents.go | Refactored agent-ID persistence to use shared helpers; clearCurrentAgentIfMatches now removes the legacy file alongside the scoped one when the IDs match, which is the documented intent |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
internal/cmd/state_scope.go:55-63
**`stateFilePath` is dead code**
`stateFilePath` is defined but never called: `readStateFile`, `writeStateFile`, and `clearStateFile` all compute the scoped path inline. This unexported helper has no callers and will generate a `go vet`/lint warning in strict configurations.
Reviews (2): Last reviewed commit: "feat: scope CLI state files by API key t..." | Re-trigger Greptile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on top of #43 (review and merge #43 first; this PR's base is
fix/parallel-sessions-start-race).Adds the B1 hardening from the original PRD: per-API-key scoping for the four CLI state files (
current_session,current_agent,current_session_expiry,current_viewer_url). Two accounts running the CLI on the same machine no longer share a single~/.notte/cli/current_sessionslot, so asessions stopfrom shell A can't accidentally target the session belonging to shell B.#43 already closed the parallel-worker race in non-TTY contexts (the urgent bug). This PR closes the related multi-account collision in interactive use, and makes a per-account workflow (e.g.
notte auth login+notte sessions startin two separate terminals) actually isolated.How it works
.<sha256(api_key)[:8]>once per process. When no API key is configured (e.g. midnotte auth login) the suffix is empty and behavior matches the legacy file paths.~/.notte/cli/current_session.abc12345.current_session.sessions.goandagents.gonow delegate all four state files to three helpers ininternal/cmd/state_scope.go:readStateFile,writeStateFile,clearStateFile. Net effect is ~70 fewer lines in the two run files.Backwards compatibility
~/.notte/cli/current_sessionkeep working: the read fallback finds it, and the next write creates the scoped copy. No migration is required and no shell session is interrupted.Test plan
go test ./...andgo vet ./...cleaninternal/cmd/state_scope_test.go:TestAPIKeyScopeSuffix_FromEnvVar- suffix is derived fromNOTTE_API_KEYand stable across callsTestStateFilePath_WithAPIKeyScope- writes land at the suffixed path and not the legacy oneTestReadStateFile_FallsBackToLegacyUnscopedPath- upgrade pathTestReadStateFile_PrefersScopedOverLegacy- precedence when both files existTestClearStateFile_RemovesBothScopedAndLegacy- no stale state after clearTestDifferentAPIKeysIsolated- two API keys read different sessions on the same machineTestMainin thecmdtest package pins the override to "" so the existing fleet ofsessions_test.go/agents_test.gocontinues to operate on the legacy unscoped pathNOTTE_API_KEY=key-A notte sessions startin shell A andNOTTE_API_KEY=key-B notte sessions startin shell B, thensessions statusin each - each shell sees its own sessionRisk / rollback
Low. The change is localized to
internal/cmd/state_scope.go+ the four helper functions insessions.go/agents.go. The legacy-read fallback covers the upgrade path; a downgrade after this lands would lose the scoped file (the older CLI would only readcurrent_session), but no session is destroyed - users just need to pass--session-iduntil theysessions startagain.